Skip to content

Improve browser cleanup, local browser PATH setup, and screenshot recovery#1001

Closed
davetist wants to merge 3 commits intoNousResearch:mainfrom
davetist:codex-browser-cleanup-fixes
Closed

Improve browser cleanup, local browser PATH setup, and screenshot recovery#1001
davetist wants to merge 3 commits intoNousResearch:mainfrom
davetist:codex-browser-cleanup-fixes

Conversation

@davetist
Copy link
Contributor

@davetist davetist commented Mar 12, 2026

Summary

This PR keeps the browser fixes that still add value on top of the latest main:

  • unify browser teardown so manual close, inactivity cleanup, and emergency cleanup share the same cleanup path
  • make local agent-browser execution more reliable by preferring the Hermes-managed Node binary in PATH
  • restore screenshot fallback recovery when agent-browser returns human-readable output instead of JSON
  • improve screenshot capture by requesting full-page screenshots and honoring the actual screenshot path returned by the CLI

What Changed

Cleanup and session lifecycle

  • changed browser_close() to delegate to cleanup_browser() instead of performing a partial inline shutdown
  • changed _emergency_cleanup_all_sessions() to route through cleanup_all_browsers() and then clear:
    • _active_sessions
    • _session_last_activity
    • _recording_sessions
  • added regression coverage for cleanup bookkeeping so the explicit close path and emergency cleanup path stay aligned

Local browser command reliability

  • shortened local session names to reduce Unix socket path pressure
  • updated _run_browser_command() to prefer $HERMES_HOME/node/bin before the standard system path entries
  • still preserves the normal system directories in PATH for minimal service environments

Screenshot and non-JSON handling

  • added _extract_screenshot_path_from_text() to recover real absolute .png paths from human-readable agent-browser output
  • changed non-JSON CLI output handling to fail clearly by default instead of treating arbitrary text as success
  • added a screenshot-specific recovery path that still returns success when a real screenshot file was created
  • updated browser_vision() to:
    • pass --full to agent-browser screenshot
    • use the actual screenshot path returned by the CLI when it differs from the requested output path

Tests

  • added tests/tools/test_browser_cleanup.py coverage for:
    • screenshot path extraction
    • cleanup_browser() state cleanup
    • browser_close() delegation
    • emergency cleanup state clearing

Not Included

  • this PR intentionally keeps the latest main behavior that removed browser signal handlers
  • it does not reintroduce the old SIGINT/SIGTERM browser cleanup hooks that were removed upstream for voice-mode stability

Verification

Passed:

  • source venv/bin/activate && python -m pytest tests/tools/test_browser_cleanup.py tests/tools/test_browser_console.py tests/gateway/test_send_image_file.py -q

Current full-suite state on this branch:

  • source venv/bin/activate && python -m pytest tests/ -q
  • this is not currently green in this checkout; it fails during collection around tests/run_interrupt_test.py with SystemExit / xdist internal errors
  • that failure is outside this PR's diff

Unify browser session teardown so manual close, inactivity cleanup, and emergency shutdown all follow the same cleanup path instead of partially duplicating logic.

This changes browser_close() to delegate to cleanup_browser(), which means recording shutdown, Browserbase release, activity bookkeeping cleanup, and local socket-directory removal now happen consistently. It also updates emergency cleanup to route through cleanup_all_browsers() and explicitly clear in-memory tracking state after teardown so stale active-session, last-activity, and recording entries are not left behind on exit.

The screenshot fallback path has also been fixed. _extract_screenshot_path_from_text() now matches real absolute PNG paths, including quoted output, so browser_vision() can recover screenshots when agent-browser emits human-readable text instead of JSON.

Regression coverage was added in tests/tools/test_browser_cleanup.py for screenshot path extraction, cleanup_browser() state removal, browser_close() delegation, and emergency cleanup state clearing.

Verified with:
- python -m pytest tests/tools/test_browser_cleanup.py -q
- python -m pytest tests/tools/test_browser_console.py tests/gateway/test_send_image_file.py -q
@davetist davetist force-pushed the codex-browser-cleanup-fixes branch from 117b984 to 73bbfdf Compare March 12, 2026 02:01
@davetist
Copy link
Contributor Author

davetist commented Mar 12, 2026

It modifies the session id too because apparently in macOS if the name is too long it doesn't work

@davetist
Copy link
Contributor Author

Added more context to the PR description

@davetist davetist changed the title Fix browser cleanup consistency and screenshot recovery Improve browser tool cleanup, PATH setup, and screenshot recovery Mar 12, 2026
@davetist davetist changed the title Improve browser tool cleanup, PATH setup, and screenshot recovery Improve browser cleanup, local browser PATH setup, and screenshot recovery Mar 14, 2026
teknium1 added a commit that referenced this pull request Mar 14, 2026
Resolve the cherry-pick against current browser_tool structure without carrying unrelated formatting churn, while preserving the intended cleanup, PATH, and screenshot recovery changes from PR #1001.
@teknium1
Copy link
Contributor

Merged via #1333. I cherry-picked your substantive browser cleanup/screenshot recovery commit onto current main, resolved it against the current browser tool structure, and kept the new cleanup + screenshot regression coverage.

@teknium1 teknium1 closed this Mar 14, 2026
@davetist
Copy link
Contributor Author

@teknium1 i love you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants